Jwir3/deleted files #3
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This adds basic deletion of files and folders, prior to other synchronization. This is implemented by using a queue of delete operations that will happen prior to copying files to/from Google drive. Once a file has been deleted, it is removed from the queue and normal synchronization resumes.
This makes main.ts a little smaller and easier to maintain.
This adds support for renaming files, which is treated as a delete operation of the old files followed by a creation of the new files.
jwir3
commented
Feb 26, 2024
| "author": "Antonio Tejada", | ||
| "authorUrl": "https://github.com/antoniotejada/", | ||
| "author": "Scott Johnson and Andrewq Tejada", | ||
| "authorUrl": "https://github.com/jwir3/", |
Author
There was a problem hiding this comment.
Meh, I didn't mean to make this change here. I can remove it if desired.
| "author": "Antonio Tejada", | ||
| "authorUrl": "https://github.com/antoniotejada/", | ||
| "author": "Scott Johnson and Andrewq Tejada", | ||
| "authorUrl": "https://github.com/jwir3/", |
Author
There was a problem hiding this comment.
Suggested change
| "authorUrl": "https://github.com/jwir3/", | |
| "authorUrl": "https://github.com/antoniotejada/", |
Owner
|
Thanks for taking the time to submit the patch, this works for simple cases but it doesn't work for the general case (and it cannot be fixed to make it work in the presence of multiple clients modifying the vault, even without conflicts), so I won't be taking it. For example:
The way to go is to have syncFolder detect deletions, see this comment https://github.com/antoniotejada/obsidian-google-drive/blob/master/main.ts#L723 I implemented that approach some time ago and I've had it working in my local copy, but want to clean it up and make it robust in the event of network disconnects and conflicts before pushing. |
Author
|
Ok, thanks for the feedback. Anything I can do to help get your version
merged?
…On Mon, Feb 26, 2024 at 12:02 PM Antonio Tejada ***@***.***> wrote:
Thanks for taking the time to submit the patch, this works for simple
cases but it doesn't work for the general case (and it cannot be fixed to
make it work in the presence of multiple clients modifying the vault, even
without conflicts), so I won't be taking it.
For example:
- if you close obsidian before synchronizing the deletequeue is lost.
- if there was an error when calling Google drive delete (network
down, auth error, etc), the deletequeue item is lost
- Doesn't take into account multiple clients modifying the vault (eg
cell phone and desktop, which is a case I'm very interested in working):
- The check between the file existing in google drive and deleting
the file from google drive is racy (one client could have deleted the file
in between).
- Deletions on one client are not pushed to another client
- Automatically resolve non-conflicts like a client deleting a file
that was later created and modified by another client
- Warn of conflicts that cannot be resolved like a client deleting
a file that has been modified in google drive since the last sync.
- Doesn't work for files in the vault deleted externally to obsidian
(this is a very minor point).
The way to go is to have syncFolder detect deletions, see this comment
https://github.com/antoniotejada/obsidian-google-drive/blob/master/main.ts#L723
I implemented that approach some time ago and I've had it working in my
local copy, but want to clean it up and make it robust in the event of
network disconnects and conflicts before pushing.
—
Reply to this email directly, view it on GitHub
<#3 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHTN4U5I2UJ7HSSGRAQ64TYVTE25AVCNFSM6AAAAABD2M5WOKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRUG44TMMBTGI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Owner
|
Thanks a bunch for the offer, unfortunately I've been distracted by shinier
things so I don't know when I'll get back to this. I could push my current
changes to a feature branch or such if you really really (really) wanted
it, but looks like that for your use case your changes are simpler and good
enough?
Your queue approach is interesting and for single client should mostly work
with a bit of error handling, but would also need persistence across
Obsidian invocations to be really robust. I thought the fact that the queue
is not sorted wrt updates could cause corruption in corner cases, but I
couldn't think of any.
On Mon, Feb 26, 2024 at 1:12 PM Scott Johnson ***@***.***>
wrote:
… Ok, thanks for the feedback. Anything I can do to help get your version
merged?
On Mon, Feb 26, 2024 at 12:02 PM Antonio Tejada ***@***.***>
wrote:
> Thanks for taking the time to submit the patch, this works for simple
> cases but it doesn't work for the general case (and it cannot be fixed
to
> make it work in the presence of multiple clients modifying the vault,
even
> without conflicts), so I won't be taking it.
>
> For example:
>
> - if you close obsidian before synchronizing the deletequeue is lost.
> - if there was an error when calling Google drive delete (network
> down, auth error, etc), the deletequeue item is lost
> - Doesn't take into account multiple clients modifying the vault (eg
> cell phone and desktop, which is a case I'm very interested in working):
> - The check between the file existing in google drive and deleting
> the file from google drive is racy (one client could have deleted the
file
> in between).
> - Deletions on one client are not pushed to another client
> - Automatically resolve non-conflicts like a client deleting a file
> that was later created and modified by another client
> - Warn of conflicts that cannot be resolved like a client deleting
> a file that has been modified in google drive since the last sync.
> - Doesn't work for files in the vault deleted externally to obsidian
> (this is a very minor point).
>
> The way to go is to have syncFolder detect deletions, see this comment
>
https://github.com/antoniotejada/obsidian-google-drive/blob/master/main.ts#L723
>
> I implemented that approach some time ago and I've had it working in my
> local copy, but want to clean it up and make it robust in the event of
> network disconnects and conflicts before pushing.
>
> —
> Reply to this email directly, view it on GitHub
> <
#3 (comment)>,
> or unsubscribe
> <
https://github.com/notifications/unsubscribe-auth/AAHTN4U5I2UJ7HSSGRAQ64TYVTE25AVCNFSM6AAAAABD2M5WOKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRUG44TMMBTGI>
> .
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
—
Reply to this email directly, view it on GitHub
<#3 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABRF2CF2H2ZOFVDPQLAQHD3YVTF7BAVCNFSM6AAAAABD2M5WOKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRUHAYTKMBUGQ>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes #1. I'm currently using it for my personal synchronization, but I'd like to upstream the patch if possible.